feat(ui,clerk-js,shared): Add TXT domain verification to self-serve SSO#8788
Open
LauraBeatris wants to merge 23 commits into
Open
feat(ui,clerk-js,shared): Add TXT domain verification to self-serve SSO#8788LauraBeatris wants to merge 23 commits into
LauraBeatris wants to merge 23 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: fe8b686 The changes in this PR will be included in the next version bump. This PR includes changesets to release 21 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds TXT-based organization domain ownership verification: new DTOs and org APIs, cache keys and internal hook with polling, OrganizationDomainsStep UI and validators, ConfigureSSO/context rewiring to pass domain mutations, appearance/localization updates across locales, and updated tests. ChangesOrganization domain TXT verification
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
|
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/hono
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/react
@clerk/react-router
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/ui
@clerk/upgrade
@clerk/vue
commit: |
LauraBeatris
force-pushed
the
laura/orgs-1594-sdk-add-txt-domain-verification-step
branch
from
094e372 to
33542f0
Compare
LauraBeatris
force-pushed
the
laura/orgs-1594-sdk-add-txt-domain-verification-step
branch
from
32ca101 to
38dd43d
Compare
LauraBeatris
force-pushed
the
laura/orgs-1594-sdk-add-txt-domain-verification-step
branch
from
38dd43d to
222c676
Compare
LauraBeatris
force-pushed
the
laura/orgs-1594-sdk-add-txt-domain-verification-step
branch
from
856c3fe to
daa721d
Compare
LauraBeatris
force-pushed
the
laura/orgs-1594-sdk-add-txt-domain-verification-step
branch
from
6505c89 to
b986eaa
Compare
LauraBeatris
force-pushed
the
laura/orgs-1594-sdk-add-txt-domain-verification-step
branch
from
9dc8975 to
e5c8252
Compare
LauraBeatris
force-pushed
the
laura/orgs-1594-sdk-add-txt-domain-verification-step
branch
from
e5c8252 to
7d0f298
Compare
LauraBeatris
force-pushed
the
laura/orgs-1594-sdk-add-txt-domain-verification-step
branch
from
7d0f298 to
d6d4cf8
Compare
LauraBeatris
force-pushed
the
laura/orgs-1594-sdk-add-txt-domain-verification-step
branch
from
6375fac to
a638361
Compare
LauraBeatris
force-pushed
the
laura/orgs-1594-sdk-add-txt-domain-verification-step
branch
from
a638361 to
d6c8f02
Compare
Contributor
API Changes Report
Summary
@clerk/sharedCurrent version: 4.17.1 Subpath
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ui/src/components/ConfigureSSO/hooks/useOrganizationEnterpriseConnection.ts (1)
159-202:⚠️ Potential issue | 🟡 MinorAdd
usertoenterpriseConnectionMutationsuseMemo dependencies
createConnectionreadsuser?.primaryEmailAddress, butuseris not in theuseMemodependency array, soconnectionNamecan be derived from a stale/undefined user.🔧 Proposed fix
- }, [organization, createEnterpriseConnection, updateEnterpriseConnection, deleteEnterpriseConnection]); + }, [user, organization, createEnterpriseConnection, updateEnterpriseConnection, deleteEnterpriseConnection]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/components/ConfigureSSO/hooks/useOrganizationEnterpriseConnection.ts` around lines 159 - 202, The useMemo creating enterpriseConnectionMutations closes over user (read in createConnection via user?.primaryEmailAddress) but user is missing from the dependency array, risking stale values; update the useMemo dependencies for enterpriseConnectionMutations to include user (so that createConnection recomputes when user or their primaryEmailAddress changes) — i.e., add user (or user.primaryEmailAddress if you prefer finer-grained deps) to the array currently containing [organization, createEnterpriseConnection, updateEnterpriseConnection, deleteEnterpriseConnection].
🧹 Nitpick comments (3)
packages/clerk-js/src/core/resources/Organization.ts (1)
294-334: ⚡ Quick winAdd JSDoc for the new public organization-domain methods before merge.
createDomain,prepareOwnershipVerification, andattemptOwnershipVerificationexpand the publicOrganizationAPI but currently ship without JSDoc. In this repo, these comments can flow into generated reference docs, so this creates docs drift risk. Please add concise@param/@returns(including bulkerrorssemantics) and flag for Docs review.As per coding guidelines,
packages/**/src/**/*.{ts,tsx}requires comprehensive JSDoc for public APIs, and public/reference-facing JSDoc changes may need Docs-team review.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/clerk-js/src/core/resources/Organization.ts` around lines 294 - 334, Add JSDoc comments to the public Organization methods createDomain, prepareOwnershipVerification, and attemptOwnershipVerification (and update bulkOwnershipVerification's visibility note if needed) describing purpose, parameters, return types, and errors semantics; include `@param` for name and params/enrollmentMode on createDomain and `@param` for domainIds on the two bulk methods, an `@returns` that documents the Promise shape including the data array and bulk errors array, and a short `@remarks` or `@public` note to flag that this is reference-facing and requires Docs-team review.Source: Coding guidelines
packages/ui/src/components/ConfigureSSO/steps/index.ts (1)
3-3: ⚡ Quick winAvoid expanding the
index.tsbarrel surface.Line 3 adds another re-export from an
index.tsbarrel. Prefer direct imports from./OrganizationDomainsStepat call sites to avoid increasing circular-dependency risk in this module graph.As per coding guidelines,
**/index.ts: “Avoid barrel files (index.ts re-exports) as they can cause circular dependencies.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/components/ConfigureSSO/steps/index.ts` at line 3, Remove the new re-export from the ConfigureSSO steps barrel: delete the line exporting OrganizationDomainsStep from this index.ts and update any call sites that currently import OrganizationDomainsStep from the barrel to import directly from './OrganizationDomainsStep'; this keeps the symbol OrganizationDomainsStep defined only in its module and avoids expanding the index.ts surface that can introduce circular-dependency risk.Source: Coding guidelines
packages/localizations/src/zh-CN.ts (1)
199-205: ConfirmorganizationDomainsSteppartials are OK: missingdomainSuggestion/domainCardfall back to en-US
packages/localizations/src/zh-CN.ts(lines 199-205) only provides a subset ofconfigureSSO.organizationDomainsStep, which is fine becauseLocalizationResourceis aDeepPartial(nested keys are optional) and@clerk/uideep-merges the user locale over the default en-US resource—so omitteddomainSuggestion/domainCardvalues will come fromenUSinstead of becomingundefined.
- Optional: add the missing
domainSuggestionanddomainCardtranslations for more complete zh-CN coverage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/localizations/src/zh-CN.ts` around lines 199 - 205, The zh-CN localization for configureSSO only includes a subset for organizationDomainsStep (title, subtitle, formFieldLabel__domain, formFieldInputPlaceholder__domain, formButtonPrimary__add), so domainSuggestion and domainCard are missing and will fall back to en-US; to fix, add the missing keys domainSuggestion and domainCard under organizationDomainsStep in the same object (provide their Chinese strings) so zh-CN fully covers configureSSO.organizationDomainsStep and avoids relying on the enUS fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/localizations/src/ar-SA.ts`:
- Around line 199-205: The localization object organizationDomainsStep is
missing the new nested keys required for TXT verification; add explicit
undefined placeholders for domainSuggestion (and its expected children if any),
domainCard, and domainCard.txtRecord inside organizationDomainsStep in each of
the three locale files (ar-SA.ts, be-BY.ts, bg-BG.ts) so the shape stays
consistent and fallback works; specifically update the organizationDomainsStep
entry to include domainSuggestion: undefined, domainCard: { txtRecord: undefined
} (and any other new nested keys) rather than omitting them.
In `@packages/localizations/src/uk-UA.ts`:
- Line 525: Change the translation entry that currently reads title: 'Verify
domains' to use the singular form for the single-domain verification screen;
locate the translation object entry containing the key/value pair title: 'Verify
domains' in packages/localizations/src/uk-UA.ts and update the string to 'Verify
domain' (or the appropriate singular Ukrainian equivalent) so the title matches
the per-domain verification subtitle.
In `@packages/localizations/src/utils/enUS_v4.ts`:
- Around line 595-597: The title for verifyDomainPage is plural ("Verify
domains") while subtitle and subtitleVerificationCodeScreen refer to a single
domain; update verifyDomainPage.title to the singular form "Verify domain" (or
alternatively make subtitle/subtitleVerificationCodeScreen plural if the flow
supports multiple domains) so the copy is consistent across
verifyDomainPage.title, verifyDomainPage.subtitle, and
verifyDomainPage.subtitleVerificationCodeScreen.
In `@packages/shared/src/react/hooks/useOrganizationDomains.tsx`:
- Around line 94-123: The mutation helpers createDomain,
prepareOwnershipVerification, and attemptOwnershipVerification currently only
call revalidate on the success path; move the revalidate call into a finally
block (or ensure it always runs on both success and error) so cache invalidation
happens even if ownership preparation/attempt throws; for createDomain
specifically, perform the create call, then try the prepareOwnershipVerification
step, and in a finally call revalidate before rethrowing any caught error
(preserving the returned/created value on success), and do the same pattern for
prepareOwnershipVerification and attemptOwnershipVerification so revalidate
always runs regardless of errors.
In `@packages/shared/src/types/organization.ts`:
- Around line 192-201: The `@returns` links for the bulk domain ownership methods
incorrectly point to the single-domain resource URL
(organization-domain-resource) while both methods return
OrganizationDomainsBulkOwnershipVerificationResource; update the two `@returns`
link targets to reference the correct bulk resource (replace the existing
https://clerk.com/docs/reference/types/organization-domain-resource link with
the correct URL for OrganizationDomainsBulkOwnershipVerificationResource) for
the prepareOwnershipVerification method and the bulk TXT-resolution method (the
one that "Resolves the published TXT record...") so the generated reference docs
match the actual return type.
In `@packages/ui/src/components/ConfigureSSO/__tests__/ConfigureSSO.test.tsx`:
- Around line 26-28: The missing-permission org test stalls because ConfigureSSO
calls __internal_useOrganizationDomains but the test (the "shows a warning if
the active organization membership lacks the manage enterprise connections
permission" case using f.withOrganizations()) only stubs
getEnterpriseConnections; fix it by calling the existing mock helper
mockOrganizationDomains(fixtures, []) (or with a non-empty domains array) in
that test before rendering so fixtures.clerk.organization.getDomains is mocked
and resolves.
In
`@packages/ui/src/components/ConfigureSSO/hooks/__tests__/useOrganizationEnterpriseConnection.test.tsx`:
- Around line 192-194: The test is using .resolves on a call that returns
undefined (not a Promise); change the assertion to assert the synchronous return
instead of using .resolves: replace await
expect(result.current.enterpriseConnectionMutations.createConnection('saml_okta',
undefined)).resolves.toBeUndefined() with a direct synchronous assertion like
expect(result.current.enterpriseConnectionMutations.createConnection('saml_okta',
undefined)).toBeUndefined() (or, if you prefer to keep a Promise-based check,
wrap the call with Promise.resolve(...) and use .resolves.toBeUndefined());
reference enterpriseConnectionMutations.createConnection in the test.
In `@packages/ui/src/components/ConfigureSSO/steps/OrganizationDomainsStep.tsx`:
- Around line 324-327: The Text span in OrganizationDomainsStep (the Text
element rendered with sx={t => ({ fontWeight: t.fontWeights.$medium, overflow:
'hidden', textOverflow: 'ellipsis' })}) lacks white-space handling so the
ellipsis can fail; update its sx style to include whiteSpace: 'nowrap' alongside
overflow: 'hidden' and textOverflow: 'ellipsis' to ensure the ellipsis renders
correctly for this non-input text node.
- Around line 110-113: The onRemove handler currently calls
domain.delete().then(() => revalidate()) and swallows any errors; update the
onRemove implementation to await domain.delete() and revalidate() inside a
try/catch (or handle .catch) so failures are caught, and surface errors to the
user (e.g., show a toast/error banner or set an error state) and optionally log
the error; ensure you still call revalidate() only on success and disable any
optimistic UI changes until the operation succeeds. Reference: onRemove handler,
domain.delete(), revalidate().
In `@packages/ui/src/components/OrganizationProfile/EnrollmentBadge.tsx`:
- Line 10: The enterprise_sso badge was added (enterprise_sso) but the render
gating still checks organizationDomain.verification.status, which misses
TXT-owned domains verified via ownershipVerification; update the gating logic in
EnrollmentBadge (where enterprise_sso is used) to check
ownershipVerification.status === 'verified' (or fallback to
organizationDomain.verification.status if needed) so the badge shows for
TXT-verified domains; modify the conditional that currently references
organizationDomain.verification.status to include ownershipVerification.status
as the authoritative check for enterprise SSO.
In `@packages/ui/src/internal/appearance.ts`:
- Around line 707-713: The appearance key
configureSSOVerifyDomainCardTxtRecordValue is declared and registered but never
applied to the rendered TXT value; either wire it into the rendered element or
remove the key and its registration. Fix by updating the RecordEntry component
where the TXT value is rendered (the Badge or the element rendering the TXT
string) to use
elementDescriptor={descriptors.configureSSOVerifyDomainCardTxtRecordValue} so
the themed element is applied, or if theming that value is unnecessary, remove
configureSSOVerifyDomainCardTxtRecordValue from the appearance contract and from
elementDescriptors to keep the contract and descriptors in sync.
---
Outside diff comments:
In
`@packages/ui/src/components/ConfigureSSO/hooks/useOrganizationEnterpriseConnection.ts`:
- Around line 159-202: The useMemo creating enterpriseConnectionMutations closes
over user (read in createConnection via user?.primaryEmailAddress) but user is
missing from the dependency array, risking stale values; update the useMemo
dependencies for enterpriseConnectionMutations to include user (so that
createConnection recomputes when user or their primaryEmailAddress changes) —
i.e., add user (or user.primaryEmailAddress if you prefer finer-grained deps) to
the array currently containing [organization, createEnterpriseConnection,
updateEnterpriseConnection, deleteEnterpriseConnection].
---
Nitpick comments:
In `@packages/clerk-js/src/core/resources/Organization.ts`:
- Around line 294-334: Add JSDoc comments to the public Organization methods
createDomain, prepareOwnershipVerification, and attemptOwnershipVerification
(and update bulkOwnershipVerification's visibility note if needed) describing
purpose, parameters, return types, and errors semantics; include `@param` for name
and params/enrollmentMode on createDomain and `@param` for domainIds on the two
bulk methods, an `@returns` that documents the Promise shape including the data
array and bulk errors array, and a short `@remarks` or `@public` note to flag that
this is reference-facing and requires Docs-team review.
In `@packages/localizations/src/zh-CN.ts`:
- Around line 199-205: The zh-CN localization for configureSSO only includes a
subset for organizationDomainsStep (title, subtitle, formFieldLabel__domain,
formFieldInputPlaceholder__domain, formButtonPrimary__add), so domainSuggestion
and domainCard are missing and will fall back to en-US; to fix, add the missing
keys domainSuggestion and domainCard under organizationDomainsStep in the same
object (provide their Chinese strings) so zh-CN fully covers
configureSSO.organizationDomainsStep and avoids relying on the enUS fallback.
In `@packages/ui/src/components/ConfigureSSO/steps/index.ts`:
- Line 3: Remove the new re-export from the ConfigureSSO steps barrel: delete
the line exporting OrganizationDomainsStep from this index.ts and update any
call sites that currently import OrganizationDomainsStep from the barrel to
import directly from './OrganizationDomainsStep'; this keeps the symbol
OrganizationDomainsStep defined only in its module and avoids expanding the
index.ts surface that can introduce circular-dependency risk.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 554ffff4-747c-4743-b949-1cbb2a53eaae
📒 Files selected for processing (91)
.changeset/curvy-hounds-fix.mdpackages/clerk-js/src/core/resources/Organization.tspackages/clerk-js/src/core/resources/OrganizationDomain.tspackages/localizations/src/ar-SA.tspackages/localizations/src/be-BY.tspackages/localizations/src/bg-BG.tspackages/localizations/src/bn-IN.tspackages/localizations/src/ca-ES.tspackages/localizations/src/cs-CZ.tspackages/localizations/src/da-DK.tspackages/localizations/src/de-DE.tspackages/localizations/src/el-GR.tspackages/localizations/src/en-GB.tspackages/localizations/src/en-US.tspackages/localizations/src/es-CR.tspackages/localizations/src/es-ES.tspackages/localizations/src/es-MX.tspackages/localizations/src/es-UY.tspackages/localizations/src/fa-IR.tspackages/localizations/src/fi-FI.tspackages/localizations/src/fr-FR.tspackages/localizations/src/he-IL.tspackages/localizations/src/hi-IN.tspackages/localizations/src/hr-HR.tspackages/localizations/src/hu-HU.tspackages/localizations/src/id-ID.tspackages/localizations/src/is-IS.tspackages/localizations/src/it-IT.tspackages/localizations/src/ja-JP.tspackages/localizations/src/kk-KZ.tspackages/localizations/src/ko-KR.tspackages/localizations/src/mn-MN.tspackages/localizations/src/ms-MY.tspackages/localizations/src/nb-NO.tspackages/localizations/src/nl-BE.tspackages/localizations/src/nl-NL.tspackages/localizations/src/pl-PL.tspackages/localizations/src/pt-BR.tspackages/localizations/src/pt-PT.tspackages/localizations/src/ro-RO.tspackages/localizations/src/ru-RU.tspackages/localizations/src/sk-SK.tspackages/localizations/src/sr-RS.tspackages/localizations/src/sv-SE.tspackages/localizations/src/ta-IN.tspackages/localizations/src/te-IN.tspackages/localizations/src/th-TH.tspackages/localizations/src/tr-TR.tspackages/localizations/src/uk-UA.tspackages/localizations/src/utils/enUS_v4.tspackages/localizations/src/vi-VN.tspackages/localizations/src/zh-CN.tspackages/localizations/src/zh-TW.tspackages/shared/src/react/hooks/index.tspackages/shared/src/react/hooks/useOrganizationDomains.shared.tspackages/shared/src/react/hooks/useOrganizationDomains.tsxpackages/shared/src/react/stable-keys.tspackages/shared/src/types/elementIds.tspackages/shared/src/types/json.tspackages/shared/src/types/localization.tspackages/shared/src/types/organization.tspackages/shared/src/types/organizationDomain.tspackages/ui/src/components/ConfigureSSO/ConfigureSSO.tsxpackages/ui/src/components/ConfigureSSO/ConfigureSSOContext.tsxpackages/ui/src/components/ConfigureSSO/ConfigureSSOWizard.tsxpackages/ui/src/components/ConfigureSSO/ResetConnectionDialog.tsxpackages/ui/src/components/ConfigureSSO/__tests__/ConfigureSSO.navigation.test.tsxpackages/ui/src/components/ConfigureSSO/__tests__/ConfigureSSO.test.tsxpackages/ui/src/components/ConfigureSSO/__tests__/ResetConnectionDialog.test.tsxpackages/ui/src/components/ConfigureSSO/domain/__tests__/organizationEnterpriseConnection.test.tspackages/ui/src/components/ConfigureSSO/domain/organizationEnterpriseConnection.tspackages/ui/src/components/ConfigureSSO/hooks/__tests__/useOrganizationEnterpriseConnection.test.tsxpackages/ui/src/components/ConfigureSSO/hooks/useOrganizationEnterpriseConnection.tspackages/ui/src/components/ConfigureSSO/steps/ConfigureStep/saml/SamlCustomConfigureSteps.tsxpackages/ui/src/components/ConfigureSSO/steps/ConfigureStep/saml/SamlGoogleConfigureSteps.tsxpackages/ui/src/components/ConfigureSSO/steps/ConfigureStep/saml/SamlMicrosoftConfigureSteps.tsxpackages/ui/src/components/ConfigureSSO/steps/ConfigureStep/saml/SamlOktaConfigureSteps.tsxpackages/ui/src/components/ConfigureSSO/steps/ConfigureStep/saml/__tests__/SamlConfigureSteps.test.tsxpackages/ui/src/components/ConfigureSSO/steps/ConfirmationStep.tsxpackages/ui/src/components/ConfigureSSO/steps/OrganizationDomainsStep.tsxpackages/ui/src/components/ConfigureSSO/steps/SelectProviderStep.tsxpackages/ui/src/components/ConfigureSSO/steps/TestConfigurationStep.tsxpackages/ui/src/components/ConfigureSSO/steps/VerifyDomainStep.tsxpackages/ui/src/components/ConfigureSSO/steps/__tests__/SelectProviderStep.test.tsxpackages/ui/src/components/ConfigureSSO/steps/__tests__/TestConfigurationStep.test.tsxpackages/ui/src/components/ConfigureSSO/steps/index.tspackages/ui/src/components/OrganizationProfile/EnrollmentBadge.tsxpackages/ui/src/components/OrganizationProfile/OrganizationSecurityPage.tsxpackages/ui/src/customizables/elementDescriptors.tspackages/ui/src/elements/contexts/index.tsxpackages/ui/src/internal/appearance.ts
💤 Files with no reviewable changes (2)
- packages/ui/src/components/ConfigureSSO/steps/VerifyDomainStep.tsx
- packages/ui/src/components/ConfigureSSO/domain/organizationEnterpriseConnection.ts
Comment on lines
+94
to
+123
| const createDomain = useCallback( | ||
| async (name: string) => { | ||
| let created = await organization?.createDomain(name, enrollmentMode ? { enrollmentMode } : undefined); | ||
|
|
||
| if (created && enrollmentMode === 'enterprise_sso') { | ||
| const prepared = await organization?.prepareOwnershipVerification([created.id]); | ||
| created = prepared?.data[0] ?? created; | ||
| } | ||
|
|
||
| await revalidate(); | ||
| return created; | ||
| }, | ||
| [organization, revalidate, enrollmentMode], | ||
| ); | ||
|
|
||
| const prepareOwnershipVerification = useCallback( | ||
| async (domains: OrganizationDomainResource[]) => { | ||
| const prepared = await organization?.prepareOwnershipVerification(domains.map(domain => domain.id)); | ||
| await revalidate(); | ||
| return prepared; | ||
| }, | ||
| [organization, revalidate], | ||
| ); | ||
|
|
||
| const attemptOwnershipVerification = useCallback( | ||
| async (domains: OrganizationDomainResource[]) => { | ||
| const attempted = await organization?.attemptOwnershipVerification(domains.map(domain => domain.id)); | ||
| await revalidate(); | ||
| return attempted; | ||
| }, |
Contributor
There was a problem hiding this comment.
Always revalidate in mutation helpers, including error paths.
createDomain can successfully persist a domain (Line 96) and then throw on ownership preparation (Line 99), but cache invalidation only runs on the success path. That leaves stale UI and makes retries misleading (domain exists while list is stale). Same risk applies to the prepare/attempt helpers.
Proposed fix
const createDomain = useCallback(
async (name: string) => {
- let created = await organization?.createDomain(name, enrollmentMode ? { enrollmentMode } : undefined);
-
- if (created && enrollmentMode === 'enterprise_sso') {
- const prepared = await organization?.prepareOwnershipVerification([created.id]);
- created = prepared?.data[0] ?? created;
- }
-
- await revalidate();
- return created;
+ try {
+ let created = await organization?.createDomain(name, enrollmentMode ? { enrollmentMode } : undefined);
+
+ if (created && enrollmentMode === 'enterprise_sso') {
+ const prepared = await organization?.prepareOwnershipVerification([created.id]);
+ created = prepared?.data[0] ?? created;
+ }
+
+ return created;
+ } finally {
+ await revalidate();
+ }
},
[organization, revalidate, enrollmentMode],
);
const prepareOwnershipVerification = useCallback(
async (domains: OrganizationDomainResource[]) => {
- const prepared = await organization?.prepareOwnershipVerification(domains.map(domain => domain.id));
- await revalidate();
- return prepared;
+ try {
+ return await organization?.prepareOwnershipVerification(domains.map(domain => domain.id));
+ } finally {
+ await revalidate();
+ }
},
[organization, revalidate],
);
const attemptOwnershipVerification = useCallback(
async (domains: OrganizationDomainResource[]) => {
- const attempted = await organization?.attemptOwnershipVerification(domains.map(domain => domain.id));
- await revalidate();
- return attempted;
+ try {
+ return await organization?.attemptOwnershipVerification(domains.map(domain => domain.id));
+ } finally {
+ await revalidate();
+ }
},
[organization, revalidate],
);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/shared/src/react/hooks/useOrganizationDomains.tsx` around lines 94 -
123, The mutation helpers createDomain, prepareOwnershipVerification, and
attemptOwnershipVerification currently only call revalidate on the success path;
move the revalidate call into a finally block (or ensure it always runs on both
success and error) so cache invalidation happens even if ownership
preparation/attempt throws; for createDomain specifically, perform the create
call, then try the prepareOwnershipVerification step, and in a finally call
revalidate before rethrowing any caught error (preserving the returned/created
value on success), and do the same pattern for prepareOwnershipVerification and
attemptOwnershipVerification so revalidate always runs regardless of errors.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Introduce a new step to create organization domains and verify them with TXT verification. This step is required before selecting a provider, and all domains must get verified before proceesing.
CleanShot.2026-06-12.at.17.12.48.mp4
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit
New Features
UX
Localization